채팅 이미지 미리보기 리팩토링#221
Open
0Jaemin0 wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthrough채팅 메시지 내 이미지 렌더링 방식을 새로운 ChatImageMessage 컴포넌트로 교체했습니다. 이미지 방향(가로/세로/정사각형)에 따른 스타일과 로드 상태 처리를 담당하는 styled-components가 추가되었고, 클릭 시 openImagePreviewWindow 유틸을 통해 새 탭에서 이미지를 미리보기로 여는 기능이 구현되었습니다. MyMessageBox와 OtherMessageBox의 기존 a/img 마크업이 이 컴포넌트로 대체되었으며, ImageWrapper에 flex-wrap이 추가되었습니다. 또한 useChatMessages의 읽음 처리 로직에 stompClient 존재 여부 검사가 추가되었습니다. Estimated code review effort: 2 (Simple) | ~15 minutes 변경 사항
관련 이슈
관련 PR
제안 라벨: enhancement, refactor, frontend 제안 리뷰어: 없음(제공된 정보 기준) 한 줄 시(리뷰어의 마음으로)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/components/Chat/ChatImageMessage/ChatImageMessage.tsx (2)
16-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win이벤트 핸들러 네이밍 컨벤션을 맞춰주세요.
updateImageMeta는onLoad에 바인딩되는 이벤트 핸들러인데, 컨벤션상handle*접두어를 사용해야 합니다.handleImageLoad등으로 변경해주세요.♻️ 제안
- const updateImageMeta = (event: React.SyntheticEvent<HTMLImageElement>) => { + const handleImageLoad = (event: React.SyntheticEvent<HTMLImageElement>) => { const { naturalWidth, naturalHeight } = event.currentTarget; ... }; ... - onLoad={updateImageMeta} + onLoad={handleImageLoad}As per path instructions, "이벤트 핸들러는 on*/handle* 규칙 준수"를 따라야 합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Chat/ChatImageMessage/ChatImageMessage.tsx` around lines 16 - 24, The onLoad image event handler in ChatImageMessage should follow the handle* naming convention instead of update*. Rename updateImageMeta to a handle-prefixed name such as handleImageLoad, and update its usage in ChatImageMessage so the event handler naming is consistent with the on*/handle* rule.Source: Path instructions
13-14: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value이미지 로드 후 카드 크기 변동으로 인한 레이아웃 점프 가능성.
orientation기본값이'portrait'(260×325)인데, 로드 완료 후 실제 방향(landscape: 260×174,square: 260×260)으로 갱신되면 카드 높이가 급격히 바뀌면서 채팅 목록에 시각적 점프가 발생할 수 있습니다.width/height에 짧은 트랜지션을 추가하면 체감 흔들림을 줄일 수 있습니다.export const ImageLink = styled.a<{...}>` ... width: ${(props) => IMAGE_CARD_SIZE[props.$orientation].width}; height: ${(props) => IMAGE_CARD_SIZE[props.$orientation].height}; + transition: width 0.15s ease, height 0.15s ease; `;Also applies to: 19-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Chat/ChatImageMessage/ChatImageMessage.tsx` around lines 13 - 14, The ChatImageMessage card can visually jump when image loading changes the `orientation` state from the default `'portrait'` to the final value. Update the sizing behavior in `ChatImageMessage` so the card transitions smoothly between the placeholder and final dimensions, especially around the `orientation` state and the rendered width/height. Add a short transition on the relevant size-related styles in the component so the height change is less abrupt after `isLoaded` flips and the actual image orientation is known.src/components/Chat/MyMessageBox/MyMessageBox.styled.ts (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ImageWrapper스타일이 OtherMessageBox.styled.ts와 완전히 중복됩니다.동일한
display: flex; flex-wrap: wrap; gap: ...블록이MyMessageBox.styled.ts와OtherMessageBox.styled.ts에 각각 정의되어 있습니다. 공용 styled 컴포넌트(예:src/components/Chat/common또는 유사 위치)로 추출해 재사용하면 이후 스타일 변경 시 두 곳을 동시에 수정할 필요가 없어집니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Chat/MyMessageBox/MyMessageBox.styled.ts` around lines 47 - 51, `ImageWrapper` in `MyMessageBox.styled.ts` is duplicated in `OtherMessageBox.styled.ts`; extract the shared flex-wrap gap styling into a common styled component in a shared chat location and update both `ImageWrapper` usages to reuse it. Keep the existing layout behavior the same while removing the duplicated `styled.div` block.src/utils/openImagePreviewWindow.ts (1)
6-18: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
noopener옵션을 window.open에 직접 지정하는 편이 더 안전합니다.현재는 빈 창을 연 뒤
opener = null로 사후 해제하는 방식인데,window.open호출 시 세 번째 인자로'noopener,noreferrer'를 넘기는 것이 더 관용적이고 확실한 방법입니다. 다만 여기서는 외부 URL로 즉시 네비게이션하지 않고 about:blank 상태에서 DOM을 직접 구성하므로 실질적인 tabnabbing 위험은 낮습니다.♻️ 제안
- const imageWindow = window.open('', '_blank'); + const imageWindow = window.open('', '_blank', 'noopener,noreferrer'); if (!imageWindow) return; - imageWindow.opener = null; imageWindow.document.title = alt;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/openImagePreviewWindow.ts` around lines 6 - 18, `openImagePreviewWindow` currently nulls `imageWindow.opener` after opening, but the safer approach is to pass the security flags directly to `window.open`. Update the `window.open` call in `openImagePreviewWindow` to include `'noopener,noreferrer'` as the third argument, and remove the manual `opener = null` handling so the window is created with the intended isolation from the start.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Chat/ChatImageMessage/ChatImageMessage.tsx`:
- Around line 27-37: The ImageLink click handler in ChatImageMessage currently
prevents the browser’s default behavior for every click, which blocks
modifier-based new tab actions. Update the onClick logic on S.ImageLink so it
only calls preventDefault() and openImagePreviewWindow({ src, alt }) for a plain
left-click, and immediately returns when a modifier key is pressed so
ctrl/cmd+click keeps the default new-tab behavior.
In `@src/hooks/chatting/useChatMessages.ts`:
- Line 17: The page merge logic in useChatMessages is tracking only the last
page via lastReadMessageIdRef, which can miss already-cached pages and duplicate
the same tail page on updates. Change the ref to track how many pages have
already been processed, and in the message merge path around the pages handling
logic use that count to append only newly received pages while preserving any
real-time prepended messages. Update the relevant merge/initialization flow in
useChatMessages so repeated React Query cache refreshes are stable and
idempotent.
- Around line 37-43: `useChatMessages`의 읽음 처리 로직에서 `stompClient`가 없을 때도
`lastReadMessageIdRef`가 갱신되어 재전송이 막히는 문제가 있습니다. `stompClient?.send`를 호출하는 부분을
`stompClient` 존재 여부를 먼저 확인한 뒤에만 실행하도록 바꾸고, 성공적으로 전송했을 때만
`lastReadMessageIdRef.current`를 `latestMessageId`로 업데이트하세요. `useChatMessages`와
`lastReadMessageIdRef`를 기준으로 해당 흐름을 찾아 수정하면 됩니다.
In `@src/hooks/chatting/useChatScroll.ts`:
- Around line 15-36: useChatScroll keeps isInitialScrollComplete and
isLatestMessageVisible across chatHistory resets, so switching rooms can reuse
stale scroll state. Update the hook’s state handling so these flags are reset
whenever chatHistory becomes null or changes to a new conversation, alongside
the existing scroll logic in useEffect/useLayoutEffect. Make the reset happen in
useChatScroll using the existing state setters and messageEndRef-based flow so
the initial bottom scroll and latest-message visibility are recalculated per
chat room.
---
Nitpick comments:
In `@src/components/Chat/ChatImageMessage/ChatImageMessage.tsx`:
- Around line 16-24: The onLoad image event handler in ChatImageMessage should
follow the handle* naming convention instead of update*. Rename updateImageMeta
to a handle-prefixed name such as handleImageLoad, and update its usage in
ChatImageMessage so the event handler naming is consistent with the on*/handle*
rule.
- Around line 13-14: The ChatImageMessage card can visually jump when image
loading changes the `orientation` state from the default `'portrait'` to the
final value. Update the sizing behavior in `ChatImageMessage` so the card
transitions smoothly between the placeholder and final dimensions, especially
around the `orientation` state and the rendered width/height. Add a short
transition on the relevant size-related styles in the component so the height
change is less abrupt after `isLoaded` flips and the actual image orientation is
known.
In `@src/components/Chat/MyMessageBox/MyMessageBox.styled.ts`:
- Around line 47-51: `ImageWrapper` in `MyMessageBox.styled.ts` is duplicated in
`OtherMessageBox.styled.ts`; extract the shared flex-wrap gap styling into a
common styled component in a shared chat location and update both `ImageWrapper`
usages to reuse it. Keep the existing layout behavior the same while removing
the duplicated `styled.div` block.
In `@src/utils/openImagePreviewWindow.ts`:
- Around line 6-18: `openImagePreviewWindow` currently nulls
`imageWindow.opener` after opening, but the safer approach is to pass the
security flags directly to `window.open`. Update the `window.open` call in
`openImagePreviewWindow` to include `'noopener,noreferrer'` as the third
argument, and remove the manual `opener = null` handling so the window is
created with the intended isolation from the start.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1aee6775-0360-46eb-ae2d-7389c496731a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.jsonsrc/components/Chat/ChatImageMessage/ChatImageMessage.styled.tssrc/components/Chat/ChatImageMessage/ChatImageMessage.tsxsrc/components/Chat/Chatting/Chatting.styled.tssrc/components/Chat/Chatting/Chatting.tsxsrc/components/Chat/MyMessageBox/MyMessageBox.styled.tssrc/components/Chat/MyMessageBox/MyMessageBox.tsxsrc/components/Chat/OtherMessageBox/OtherMessageBox.styled.tssrc/components/Chat/OtherMessageBox/OtherMessageBox.tsxsrc/hooks/chatting/useChatInfiniteScroll.tssrc/hooks/chatting/useChatInput.tssrc/hooks/chatting/useChatMessages.tssrc/hooks/chatting/useChatScroll.tssrc/hooks/common/useIntersectionObserver.tssrc/hooks/queries/chat/useChatMessageQuery.tssrc/utils/openImagePreviewWindow.ts
| const [chatHistory, setChatHistory] = useState<ChatData | null>(null); | ||
| const chatRef = useRef<HTMLDivElement>(null); | ||
| const [paginationVersion, setPaginationVersion] = useState(0); | ||
| const lastReadMessageIdRef = useRef<number | null>(null); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
페이지 병합 기준을 “마지막 페이지”가 아니라 “새로 처리한 페이지”로 추적하세요.
React Query 캐시에 여러 pages가 이미 있으면 Line 50-53은 마지막 페이지만 넣어 최신 페이지를 누락할 수 있고, 같은 페이지 배열이 갱신되면 마지막 페이지를 중복 append할 수 있습니다. 처리한 page count를 ref로 관리하면 실시간으로 prepend된 메시지는 보존하면서 초기 캐시/추가 페이지를 안정적으로 병합할 수 있습니다.
수정 예시
const [paginationVersion, setPaginationVersion] = useState(0);
const lastReadMessageIdRef = useRef<number | null>(null);
+ const loadedPageCountRef = useRef(0);
@@
setChatHistory(null);
setPaginationVersion(0);
lastReadMessageIdRef.current = null;
+ loadedPageCountRef.current = 0;
}, [selectedChat]);
@@
useEffect(() => {
if (!messagePages) return;
+ const nextPages = messagePages.slice(loadedPageCountRef.current);
+ if (nextPages.length === 0) return;
+
+ const nextMessages = nextPages.flatMap((page) => page.chatChannelMessages);
+ loadedPageCountRef.current = messagePages.length;
+
setChatHistory((prevChatHistory) => {
- const lastPageMessages = messagePages[messagePages.length - 1]?.chatChannelMessages ?? [];
-
return {
- chatChannelMessages: [...(prevChatHistory?.chatChannelMessages ?? []), ...lastPageMessages],
+ chatChannelMessages: [...(prevChatHistory?.chatChannelMessages ?? []), ...nextMessages],
};
});
setPaginationVersion((version) => version + 1);
}, [messagePages]);As per path instructions, API 호출, 상태 관리(Zustand, React Query), 컴포넌트 구조가 적절하게 분리되어 있는지 확인해주세요.
Also applies to: 25-29, 46-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/chatting/useChatMessages.ts` at line 17, The page merge logic in
useChatMessages is tracking only the last page via lastReadMessageIdRef, which
can miss already-cached pages and duplicate the same tail page on updates.
Change the ref to track how many pages have already been processed, and in the
message merge path around the pages handling logic use that count to append only
newly received pages while preserving any real-time prepended messages. Update
the relevant merge/initialization flow in useChatMessages so repeated React
Query cache refreshes are stable and idempotent.
Source: Path instructions
Comment on lines
15
to
+36
| const [isScrollAtBottom, setIsScrollAtBottom] = useState(false); | ||
| const [initialLoad, setInitialLoad] = useState(true); | ||
| const [isInitialScrollComplete, setIsInitialScrollComplete] = useState(false); | ||
| const [isLatestMessageVisible, setIsLatestMessageVisible] = useState(false); | ||
| const messageEndRef = useRef<HTMLInputElement | null>(null); | ||
| const messageEndRef = useRef<HTMLDivElement | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (!chatHistory || chatHistory.chatChannelMessages.length === 0) return; | ||
|
|
||
| if (isScrollAtBottom) { | ||
| messageEndRef.current?.scrollIntoView(); | ||
| setIsScrollAtBottom(false); | ||
| } else if (chatRef.current) | ||
| window.scrollTo({ top: chatRef.current.scrollHeight - prevHeight }); | ||
| }, [chatHistory]); | ||
| if (!chatHistory || chatHistory.chatChannelMessages.length === 0 || !isScrollAtBottom) { | ||
| return; | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| const observedElement = chatRef.current; | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| if (initialLoad) { | ||
| messageEndRef.current?.scrollIntoView(); | ||
| setInitialLoad(false); | ||
| } | ||
| }); | ||
| messageEndRef.current?.scrollIntoView(); | ||
| setIsScrollAtBottom(false); | ||
| }, [chatHistory, isScrollAtBottom]); | ||
|
|
||
| if (observedElement) resizeObserver.observe(observedElement); | ||
| useLayoutEffect(() => { | ||
| if (isInitialScrollComplete || !chatHistory || chatHistory.chatChannelMessages.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| return () => { | ||
| if (observedElement) resizeObserver.unobserve(observedElement); | ||
| }; | ||
| }, []); | ||
| messageEndRef.current?.scrollIntoView(); | ||
| setIsInitialScrollComplete(true); | ||
| }, [chatHistory, isInitialScrollComplete]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
채팅 히스토리 초기화 시 스크롤 상태도 함께 초기화하세요.
useChatMessages는 채팅방 변경 시 chatHistory를 null로 초기화하지만, 이 훅의 isInitialScrollComplete/isLatestMessageVisible은 유지됩니다. 다음 채팅방에서 초기 하단 스크롤이 스킵되거나 이전 방의 최신 메시지 버튼 상태가 남을 수 있습니다.
수정 예시
useLayoutEffect(() => {
- if (isInitialScrollComplete || !chatHistory || chatHistory.chatChannelMessages.length === 0) {
+ if (!chatHistory) {
+ setIsScrollAtBottom(false);
+ setIsInitialScrollComplete(false);
+ setIsLatestMessageVisible(false);
+ return;
+ }
+
+ if (isInitialScrollComplete || chatHistory.chatChannelMessages.length === 0) {
return;
}
messageEndRef.current?.scrollIntoView();As per path instructions, 불필요한 상태 관리와 null/undefined 처리, 에러 처리 등 안정성 관점을 기준으로 확인했습니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [isScrollAtBottom, setIsScrollAtBottom] = useState(false); | |
| const [initialLoad, setInitialLoad] = useState(true); | |
| const [isInitialScrollComplete, setIsInitialScrollComplete] = useState(false); | |
| const [isLatestMessageVisible, setIsLatestMessageVisible] = useState(false); | |
| const messageEndRef = useRef<HTMLInputElement | null>(null); | |
| const messageEndRef = useRef<HTMLDivElement | null>(null); | |
| useEffect(() => { | |
| if (!chatHistory || chatHistory.chatChannelMessages.length === 0) return; | |
| if (isScrollAtBottom) { | |
| messageEndRef.current?.scrollIntoView(); | |
| setIsScrollAtBottom(false); | |
| } else if (chatRef.current) | |
| window.scrollTo({ top: chatRef.current.scrollHeight - prevHeight }); | |
| }, [chatHistory]); | |
| if (!chatHistory || chatHistory.chatChannelMessages.length === 0 || !isScrollAtBottom) { | |
| return; | |
| } | |
| useEffect(() => { | |
| const observedElement = chatRef.current; | |
| const resizeObserver = new ResizeObserver(() => { | |
| if (initialLoad) { | |
| messageEndRef.current?.scrollIntoView(); | |
| setInitialLoad(false); | |
| } | |
| }); | |
| messageEndRef.current?.scrollIntoView(); | |
| setIsScrollAtBottom(false); | |
| }, [chatHistory, isScrollAtBottom]); | |
| if (observedElement) resizeObserver.observe(observedElement); | |
| useLayoutEffect(() => { | |
| if (isInitialScrollComplete || !chatHistory || chatHistory.chatChannelMessages.length === 0) { | |
| return; | |
| } | |
| return () => { | |
| if (observedElement) resizeObserver.unobserve(observedElement); | |
| }; | |
| }, []); | |
| messageEndRef.current?.scrollIntoView(); | |
| setIsInitialScrollComplete(true); | |
| }, [chatHistory, isInitialScrollComplete]); | |
| const [isScrollAtBottom, setIsScrollAtBottom] = useState(false); | |
| const [isInitialScrollComplete, setIsInitialScrollComplete] = useState(false); | |
| const [isLatestMessageVisible, setIsLatestMessageVisible] = useState(false); | |
| const messageEndRef = useRef<HTMLDivElement | null>(null); | |
| useEffect(() => { | |
| if (!chatHistory || chatHistory.chatChannelMessages.length === 0 || !isScrollAtBottom) { | |
| return; | |
| } | |
| messageEndRef.current?.scrollIntoView(); | |
| setIsScrollAtBottom(false); | |
| }, [chatHistory, isScrollAtBottom]); | |
| useLayoutEffect(() => { | |
| if (!chatHistory) { | |
| setIsScrollAtBottom(false); | |
| setIsInitialScrollComplete(false); | |
| setIsLatestMessageVisible(false); | |
| return; | |
| } | |
| if (isInitialScrollComplete || chatHistory.chatChannelMessages.length === 0) { | |
| return; | |
| } | |
| messageEndRef.current?.scrollIntoView(); | |
| setIsInitialScrollComplete(true); | |
| }, [chatHistory, isInitialScrollComplete]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/chatting/useChatScroll.ts` around lines 15 - 36, useChatScroll
keeps isInitialScrollComplete and isLatestMessageVisible across chatHistory
resets, so switching rooms can reuse stale scroll state. Update the hook’s state
handling so these flags are reset whenever chatHistory becomes null or changes
to a new conversation, alongside the existing scroll logic in
useEffect/useLayoutEffect. Make the reset happen in useChatScroll using the
existing state setters and messageEndRef-based flow so the initial bottom scroll
and latest-message visibility are recalculated per chat room.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈
✨ PR 세부 내용
채팅 메시지의 이미지 표시와 미리보기 동작을 별도 컴포넌트와 유틸로 분리했습니다.
기존에는 이미지가 단순
<img>링크로만 렌더링되어 있었기 때문에, 이미지 크기와 미리보기 동작을 메시지 타입별로 일관되게 제어하기 어려웠습니다. 이번 작업에서는 채팅 이미지 표시 책임을 분리하고, 클릭 시 새 창 미리보기를 통일된 방식으로 처리하도록 정리했습니다.핵심 변경
openImagePreviewWindow유틸 추가ChatImageMessage컴포넌트 추가MyMessageBox/OtherMessageBox에서 이미지 렌더링 로직 교체구조 개선
효과
📸 스크린샷
✅ 리뷰 요구사항
Summary by CodeRabbit
New Features
Bug Fixes